-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Enforce in bootstrap that doc must have stage at least 1 #145011
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
☔ The latest upstream changes (presumably #145020) made this pull request unmergeable. Please resolve the merge conflicts. |
This PR modifies If appropriate, please update |
Rebased over the two merged PRs, should be ready now. @rustbot ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! The changes look good in the general sense that it does feel more self-consistent. I do have some questions though that occurred to me when looking at the changes.
#[test] | ||
#[should_panic] | ||
fn doc_compiler_stage_0() { | ||
let ctx = TestCtx::new(); | ||
insta::assert_snapshot!( | ||
ctx.config("doc") | ||
.path("compiler") | ||
.stage(0) | ||
.render_steps(), @r" | ||
[build] rustdoc 0 <host> | ||
[build] llvm <host> | ||
[doc] rustc 0 <host> | ||
"); | ||
ctx.config("doc").path("compiler").stage(0).run(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: hm, would we regress any actual use cases of documenting the stage 0 compiler?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't document the stage 0 compiler, because we don't have its source code. My mental model around this looks like this:
- Every stage has two components attached to it: sources and artifacts.
- We do not have the source of stage 0 available.
- When we do check/clippy/build/doc, we always work with sources, not artifacts, so it makes no sense for it to work on stage 0.
- When we do test, we work with artifacts, so it makes sense (in niche scenarios) to also run it on stage 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that makes sense 👍. I.e. source-availability vs artifact-availability.
// Build rustc docs so that we generate relative links. | ||
run.builder.ensure(Rustc::from_build_compiler(run.builder, compilers.build_compiler(), target)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remark: hah, this is not super obvious...
[doc] rustc 2 <host> -> std 2 <host> crates=[] | ||
[doc] rustc 2 <host> -> std 2 <target1> crates=[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remark: hm, this is always a bit funny, when we run a std doc step with
crates=[alloc,compiler_builtins,core,panic_abort,panic_unwind,proc_macro,rustc-std-workspace-core,std,std_detect,sysroot,test,unwind]
but then again later with
crates=[]
then I think we rely on the fact that they can unify to not have this invalidate the previous build cache (?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think so. I did some experiments with x clippy compiler
steps with and without unification and it looked like the cache is not being broken, but we'll see I guess.
[doc] book (book) <host> | ||
[doc] book/first-edition (book) <host> | ||
[doc] book/second-edition (book) <host> | ||
[doc] book/2018-edition (book) <host> | ||
[build] rustdoc 1 <host> | ||
[doc] rustc 1 <host> -> standalone 2 <host> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: wait, are we building less than we should be? I.e. there are two hosts,
Hm... or taking a step back, what does the "hosts" config actually mean in bootstrap?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a very good question 😆 When you specify hosts
, you will essentially override the default host build target, see this:
rust/src/bootstrap/src/core/config/config.rs
Line 859 in d8dca72
config.hosts = if let Some(TargetSelectionList(arg_host)) = flags_host { |
In terms of what hosts means, the documentation is here:
Line 218 in 213d946
hosts: Vec<TargetSelection>, |
hosts
determines for what targets we should build host tools, while targets
determines for what targets we should build the stdlib.
But yeah, it's super confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argh, I think I asked you about this once already, but I keep forgetting the actual distinction.
@rustbot author |
@rustbot ready |
As they were previously.
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 53af067 (parent) -> a6620a4 (this PR) Test differencesShow 7 test diffsStage 0
Additionally, 4 doctest diffs were found. These are ignored, as they are noisy. Job group index Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard a6620a45bd29575cce67b6a0ab2956aef105e324 --output-dir test-dashboard And then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
Finished benchmarking commit (a6620a4): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (secondary -3.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 3.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 463.403s -> 464.586s (0.26%) |
Enable RUST_BACKTRACE=1 on CI We should really see the backtrace if something in bootstrap fails on CI. #145011 (comment) doesn't show many details.
Hmm, the |
#145253 should fix it. |
So the previous meaning of stage1 (build rustdoc with current source and beta compiler) has been erased? This would be a pretty big change, eliminating certain workflows but also meaning the cc @GuillaumeGomez Does this unblock any of your PRs? |
Could you please clarify what do you mean by "previous meaning of stage1"? This PR shouldn't really make any large changes to rustdoc workflow. It just makes staging of |
I think @lolbinarycat is talking about the |
right, because |
I still have to get to fixing test steps, so far I mostly fixed build, check, tools, doc, codegen backends and now working on Clippy. Then I'll work on dist and test and it should be mostly done. |
…ieyouxu Document compiler and stdlib in stage1 in `pr-check-2` CI job This restores the original behavior pre-rust-lang#145011 (I thought that stage 2 makes more sense here, but it made the job ~30m slower, which is bad). Let's see what will be the "new" duration, it should be ~55 minutes. r? `@jieyouxu`
…ieyouxu Document compiler and stdlib in stage1 in `pr-check-2` CI job This restores the original behavior pre-rust-lang#145011 (I thought that stage 2 makes more sense here, but it made the job ~30m slower, which is bad). Let's see what will be the "new" duration, it should be ~55 minutes. r? ``@jieyouxu``
…ieyouxu Document compiler and stdlib in stage1 in `pr-check-2` CI job This restores the original behavior pre-rust-lang#145011 (I thought that stage 2 makes more sense here, but it made the job ~30m slower, which is bad). Let's see what will be the "new" duration, it should be ~55 minutes. r? ```@jieyouxu```
…ieyouxu Document compiler and stdlib in stage1 in `pr-check-2` CI job This restores the original behavior pre-rust-lang#145011 (I thought that stage 2 makes more sense here, but it made the job ~30m slower, which is bad). Let's see what will be the "new" duration, it should be ~55 minutes. r? ````@jieyouxu````
…ieyouxu Document compiler and stdlib in stage1 in `pr-check-2` CI job This restores the original behavior pre-rust-lang#145011 (I thought that stage 2 makes more sense here, but it made the job ~30m slower, which is bad). Let's see what will be the "new" duration, it should be ~55 minutes. r? `````@jieyouxu`````
…ieyouxu Document compiler and stdlib in stage1 in `pr-check-2` CI job This restores the original behavior pre-rust-lang#145011 (I thought that stage 2 makes more sense here, but it made the job ~30m slower, which is bad). Let's see what will be the "new" duration, it should be ~55 minutes. r? ``````@jieyouxu``````
Following with the bootstrap cleanups, this time around
doc
steps. Should be pretty straightforward, because the supporting infrastructure was already there.The only thing I found a bit fishy is using
Mode::ToolBootstrap
as a "catch-all" mode for non-rustc-private steps intool_doc!
, but I don't think that we need to distinguish the tools in some special way when documenting them, apart from supportingrustc_private
.Before,
x doc
more or less defaulted to what we call stage 2 now. Now it is properly stage 1, so e.g.x doc compiler
documents the compiler using the stage0/beta rust(do)c.r? @jieyouxu
try-job: dist-aarch64-msvc